Conversation
lib/ZInit.c
Outdated
| #if defined(__APPLE__) && defined(__MACH__) | ||
| if (__My_addr.s_addr == INADDR_NONE) { | ||
| nwi_state_t state; | ||
| state = nwi_state_copy(); |
There was a problem hiding this comment.
Why not merge these two lines?
There was a problem hiding this comment.
No good reason other than trying to copy local style.
| @@ -0,0 +1,286 @@ | |||
| /* | |||
There was a problem hiding this comment.
Why is there an Apple header included? Is this not part of the macOS SDK?
There was a problem hiding this comment.
It's not; the header is distributed in a separate tarball. The symbols are, however, part of the SDK libraries.
There was a problem hiding this comment.
I'm not enthusiastic about having to ship an Apple header for macOS support, but the licence is permissive, so I guess it's fine.
| if (code) | ||
| fprintf (stderr, "%s: %s: %s\n", | ||
| argv[0], error_message (code), ssline); | ||
| ZClosePort(); |
There was a problem hiding this comment.
Why? Is this just to close the UPnP session before we exit?
There was a problem hiding this comment.
Yes, otherwise the UPnP port mapping will persist on the router until you reboot the router.
There was a problem hiding this comment.
This is really making me want to have a proper zephyr_ctx that we can manage the lifecycle with. It's a bit annoying we can't easily test for these sorts of port leaks. Not something to address now, of course.
lib/ZSubs.c
Outdated
| int size, start, numok; | ||
| Z_AuthProc cert_routine; | ||
|
|
||
| if (ZGetFD() < 0) |
There was a problem hiding this comment.
Please add {} to all new ifs. braceless if statements should be discouraged.
There was a problem hiding this comment.
This was copied and pasted verbatim from a different file that didn't have curly braces. I added them.
| __UPnP_active = UPNP_GetIGDFromUrl(__UPnP_rooturl, &__UPnP_urls, &__UPnP_data, NULL, 0); | ||
| } | ||
| if (__UPnP_active) { | ||
| char port_str[16]; |
There was a problem hiding this comment.
Why 16? Port are only 0..65535, so shouldn't [8] be sufficient?
There was a problem hiding this comment.
This is copied verbatim from the library.
| } | ||
| if (__UPnP_active) { | ||
| char port_str[16]; | ||
| snprintf(port_str, 16, "%d", ntohs(__Zephyr_port)); |
There was a problem hiding this comment.
Error checking to make sure snprintf didn't overflow?
|
I'd like at least one other person (@kaduk, perhaps?) to review before we merge this one. |
2 participants
This patch series uses UPnP to make Zephyr work transparently behind a consumer-grade NAT that supports UPnP-IGD.